-
Notifications
You must be signed in to change notification settings - Fork 981
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(test): Sentinel Integration Test #728
Conversation
Needed for sentinel support (#706) Signed-off-by: ashotland <ari@dragonflydb.io>
Signed-off-by: ashotland <ari@dragonflydb.io>
Signed-off-by: ashotland <ari@dragonflydb.io>
Signed-off-by: ashotland <ari@dragonflydb.io>
async def get(): return await master_client.get("key") # There is no async lambda | ||
await await_for( | ||
get, | ||
lambda val: val == b"value", | ||
10, "Timeout waiting for key to exist in replica." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes generic utils and async don't mix well in Python. You could've used lambdas if you made them return coroutines which are awaited internally. Or not used the util at all:
async with asyncio.timeout(10):
while (await master_client.get('key')) != b"value":
await asyncio.sleep(1.0)
But it doesn't matter as its just tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, returning a coroutine makes sense to me, I'll merge this version and try that later.
def run_cmd(self, args, sentinel_cmd=True, capture_output=False, assert_ok=True) -> subprocess.CompletedProcess: | ||
run_args = ["redis-cli", "-p", f"{self.port}"] | ||
if sentinel_cmd: run_args = run_args + ["sentinel"] | ||
run_args = run_args + args | ||
cp = subprocess.run(run_args, capture_output=capture_output, text=True) | ||
if assert_ok: | ||
assert cp.returncode == 0, f"Command failed: {run_args}" | ||
return cp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure the default client libraries support sentinel commands which would have also saved you from writing stdout_as_list_of_dicts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a brief look at the client library API it does't seem to cover all commands I've used here and I think we'd want to be able to query the internal state of the sentinel to better asses the integration.
No description provided.